-
-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow configuring relay-proxy request log verbosity #1946
feat: allow configuring relay-proxy request log verbosity #1946
Conversation
✅ Deploy Preview for go-feature-flag-doc-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allow select verbosity of relay proxy request logging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @fals,
thanks a lot for this PR and I think you are right but we should probably be better in log management in general.
Instead of the VerboseRequestLogging
I would prefer to have a property called LogLevel
to set the level of the logs we want for the application.
The levels should be the classic ones debug
, info
, warn
, error
.
By setting this new option, we can also maybe deprecate the debug
that is a bit too trivial here.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1946 +/- ##
==========================================
+ Coverage 86.81% 86.88% +0.07%
==========================================
Files 98 98
Lines 3526 3553 +27
==========================================
+ Hits 3061 3087 +26
- Misses 357 358 +1
Partials 108 108 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the time it took for me to review it.
I added some comments about:
- the removal of the
debug
option. - the way you init
zap
. - the name of the option that is not using camelcase in the config file.
Despite that it looks super good 👍
|
@@ -48,37 +48,40 @@ func main() { | |||
_ = f.Parse(os.Args[1:]) | |||
|
|||
// Init logger | |||
zapLog := log.InitLogger() | |||
defer func() { _ = zapLog.Sync() }() | |||
logger := log.InitLogger() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomaspoignant any tip to satisfy this coverage? The test for main is empty, just a dummy test there, and all new code I introduced is flagged, so I believe I have to find where exactly you test this cases. Could you guide me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the time I don't test the main function.
So I guess this is not really a problem here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomaspoignant let me know then what else needs to change to push this forward. The codecoverage isn't passing, that's why I asked the question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have spotted 2 minor things and I have pushed 1 more commit.
I will merge the PR.
Thanks a lot @fals for your contribution, this will be available in the next version of GO Feature Flag.
To continue improving the logging of GOFF I have also opened this PR #1955 that will support levelled log inside the core go module of GOFF.
cmd/relayproxy/config/config.go
Outdated
if proxyConf.Debug && proxyConf.LogLevel == "" { | ||
log.Warn( | ||
"Option Debug that you are using in your configuration file is deprecated" + | ||
"and will be removed in future versions." + | ||
"Please use logLevel: debug to continue to run the relay-proxy with debug logs.") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proxyConf.LogLevel == ""
will never be true because you have a default value.
if proxyConf.Debug && proxyConf.LogLevel == "" { | |
log.Warn( | |
"Option Debug that you are using in your configuration file is deprecated" + | |
"and will be removed in future versions." + | |
"Please use logLevel: debug to continue to run the relay-proxy with debug logs.") | |
} | |
if proxyConf.Debug { | |
log.Warn( | |
"Option Debug that you are using in your configuration file is deprecated " + | |
"and will be removed in future versions." + | |
"Please use logLevel: debug to continue to run the relay-proxy with debug logs.") | |
} |
cmd/relayproxy/config/config.go
Outdated
if c == nil { | ||
return false | ||
} | ||
return c.LogLevel == "debug" || c.Debug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking in lowercase to be ready for any configuration string.
return c.LogLevel == "debug" || c.Debug | |
return strings.ToLower(c.LogLevel) == "debug" || c.Debug |
Signed-off-by: Thomas Poignant <[email protected]>
Quality Gate passedIssues Measures |
Description
TL;DR;
We are using the
relay-proxy
for a while in our system and we noticed that every request to relay proxy is recorded as log with level=info. Currently this log became a problem, because we generate 1.8 million request every 5 min to relay-proxy. The amount of logs injected in increase as consequence storage and cost.This PR implements configuring the verbosity of
relay-proxy
, allowing you to log more if needed, or only errors.Checklist
README.md
and/website/docs
)